It looks like the valgrind and LLVM tool patches were only just merged in the last couple months - are they included in published releases? It'll probably take at least a year or two for them to get into e.g. LTS releases on distros, which we may not want to wait for, but having at least some release seems reasonable to me and is probably not that long a wait.

@bors try @rust-timer queue

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

⌛ Trying commit 4f1bf2a with merge d6cf5a91f80fba50e1594bddce52ee42486468c6...

It looks like the valgrind and LLVM tool patches were only just merged in the last couple months - are they included in published releases? It'll probably take at least a year or two for them to get into e.g. LTS releases on distros, which we may not want to wait for, but having at least some release seems reasonable to me and is probably not that long a wait.

As far as I can tell, the LLVM patch is in LLVM 13 (GitHub says llvm/llvm-project@0a2d4f3 is on the branch at least) and valgrind 3.18.0 has support (release notes).

☀️ Try build successful - checks-actions
Build commit: d6cf5a91f80fba50e1594bddce52ee42486468c6 (d6cf5a91f80fba50e1594bddce52ee42486468c6)

Queued d6cf5a91f80fba50e1594bddce52ee42486468c6 with parent af9b508, future comparison URL.

Finished benchmarking commit (d6cf5a91f80fba50e1594bddce52ee42486468c6): comparison url.

Summary: This change led to very large relevant mixed results 🤷 in compiler performance.

• Very large improvement in instruction counts (up to -20.7% on incr-patched: Job builds of regex)
• Very large regression in instruction counts (up to 9.5% on incr-unchanged builds of webrender-wrench)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

the LLVM patch is in LLVM 13 [...] and valgrind 3.18.0 has support (release notes).

FTR, on the just released Ubuntu 21.10, the llvm available in the repos is 13.0, and the valgrind available is 3.17.0. Debian sid however is still on LLVM 11.0.

Nix OS has just updated to the new valgrind on their master branch, and I think it'll take a few days until it arrives on hydra. Stable users on 21.05 still get 3.16.1, but 21.11 seems to be on track to getting the new version.

As discussed in the compiler team triage meeting a while ago, we will do a @rust-lang/compiler team FCP on this PR. That will take at least 10 days, so that the new default would make it to stable 1.58 (released January 13th 2022). With that timeline in mind, do you still think it is necessary to wait for a valgrind release, @Mark-Simulacrum?

I think the latest valgrind release already contains the relevant patches. I'm not sure how I feel about the default being not supported by tooling on LTS distros, but I don't know that we want to wait for the potentially long time that would take -- it seems unlikely to be too impactful -- rustfilt is pretty fast and easy to install. I think we can see how much trouble we might run into by flipping the default for the compiler itself now (perhaps even before an FCP concludes) so that we can get some sense of the impact -- to maximize the time for identifying any problems we can observe in the wild from users and compiler developers using these.

Another concern I'll note that as of now this increases the compiler's install size by ~27 MB (from 378 to 405 MB) -- not a huge increase, but also not particularly small. The download size only goes up by ~5MB so I guess the extended symbols compress well. The stripped binaries avoid this extra cost, but those aren't what we ship today. Maybe it's worth blocking this on stable support for split-debuginfo, which would give a smoother path toward stripping binaries (right?) for most users.

But this is not any hard blockers IMO, just some concerns to think through as we make this decision. I think a writeup of each of these and why we're making a particular tradeoff here would be helpful to me at least. On the other hand, there are definite benefits to the new mangling -- e.g., I've been using it for some of the performance triage + improvement work I'm doing locally -- so generally I think we should move ahead.

Timeline wise, I don't know whether we expect much breakage or issues filed, but I'll note that shipping in early January probably means most of the beta/nightly cycles will be during holidays and generally we don't have as much speed. So maybe delaying 1 releases (to ~1.59 or 1.60) would be of benefit regardless.

I don't think split-debuginfo can handle linking (import/export) symbols - and I've only heard of a way to split the latter on Windows (mapping names to "ordinals").

^{(I sometimes "symbols" used ambiguously - the distinction I'm making is that debuginfo is non-semantic, but "linking names" are semantic and allow less wiggle room)}

It would be great to keep linking names compressed (esp. since a lot of names would share very similar crate roots, path components, etc.) until some rustc-dev-using project needs to link to librustc_driver.so, or rustc ICEs and wants to print a backtrace without debuginfo available.

Sadly I'm worried platform tooling is decades behind having this kind of infrastructure directly supported, and we'd have to do a lot of it ourselves (and in brittle ways).

I think we can see how much trouble we might run into by flipping the default for the compiler itself now (perhaps even before an FCP concludes) so that we can get some sense of the impact

Yes, that sounds like a good idea. I'll open a PR shortly.

I think a writeup of each of these and why we're making a particular tradeoff here would be helpful to me at least.

What format would you like that to have? Extending the PR message maybe?

On the other hand, there are definite benefits to the new mangling

I think that's an important point: there's also a cost to keeping the old mangling scheme around. We developed the new scheme because the old one is lacking in a number of ways.

shipping in early January probably means most of the beta/nightly cycles will be during holidays

That's good point.

I think we can see how much trouble we might run into by flipping the default for the compiler itself now (perhaps even before an FCP concludes) so that we can get some sense of the impact

Implemented in PR #90054.

Yeah, I was thinking about adding to the PR message. I skimmed the RFC but I think it didn't directly address the tradeoffs I mentioned.

Another aspect which seems like it's worth including is our plans for eventual removal of the old mangling scheme. I could also imagine that some users might be interested in an alternate mangling that is just a hash and so is as short as possible.

I don't think split-debuginfo can handle linking (import/export) symbols - and I've only heard of a way to split the latter on Windows (mapping names to "ordinals").

Hm, I guess. I think there's probably still an increase in debuginfo size with the new mangling but I suppose it's probably not as important. Seems good to call out this support / lack thereof, though.

Another aspect which seems like it's worth including is our plans for eventual removal of the old mangling scheme. I could also imagine that some users might be interested in an alternate mangling that is just a hash and so is as short as possible.

Yes, we might want to extend the new scheme with a well-defined "hash mode" for cases like this. E.g. _RH<sha256 of what the symbol_name would be in non-hash mode>.

I would also be interested in a "zstd mode" with a predefined dictionary. That might give additional compression without losing information. But we'd need to collect numbers for that and make sure zstd dictionaries are standardized enough for something like that.

But both things would need a proper RFC amendment.

I've labeled the PR as blocked for now because of @Mark-Simulacrum's point about this becoming stable right after the holiday season. I'll look into a writeup about tradeoffs some time next week.

Timeline wise, I don't know whether we expect much breakage or issues filed, but I'll note that shipping in early January probably means most of the beta/nightly cycles will be during holidays and generally we don't have as much speed. So maybe delaying 1 releases (to ~1.59 or 1.60) would be of benefit regardless.

I think this is a really good point! Perhaps it would make sense to land this change on nightly and disable it once beta branches for a few cycles? That would give us extra testing time on nightly while not putting us on a path toward shipping in 1.58 when many people would have been on holiday.

It looks like valgrind may also not support the v0 format just yet.

I hit this yesterday as well. The one-line fix was enough for things to work for me (modulo the small number of symbols with the .llvm.<numbers> suffixes, which are a separate issue). It appears that support for v0 demangling was added to Valgrind, but the single incorrect condition in the outer layer of Valgrind's demangling code meant that the newly added code is never run. So it certainly seems like it was never tested properly, and https://bugs.kde.org/show_bug.cgi?id=431306 didn't add any tests to Valgrind.

I will fix the problem on the Valgrind side, including adding some tests.

It looks like Valgrind issues a release about once a year? Oof, bad timing. Still, I suppose that gives more time for testing...

https://bugs.kde.org/show_bug.cgi?id=445184 is for the Valgrind fix.

The '.llvm.' suffix issue is important, IMO. I think that the v0 spec doesn't allow . chars. But if LLVM is going to add such suffixes, it seems like it needs to, in some fashion.

@michaelwoerister said:

Here is a link to the relevant Itanium mangling section: https://itanium-cxx-abi.github.io/cxx-abi/abi.html#mangling-structure
I wonder if just adding something like that to the grammar would be sufficient to consider the problem solved. I'd certainly prefer if LLVM and other backends wouldn't just append random stuff to symbol names.

Discussed on 2022-01-13 during T-compiler meeting (see Zulip thread)

@rustbot label -I-compiler-nominated

I think this can be revived, though I'm not sure what the conclusion was regarding how long we should wait for external tools to pick up and distribute the (now fixed) v0.

I like the aforementioned idea of having a "hash-only" symbol mode for performance; maybe we could even have Cargo automatically turn it on when the "strip" profile option is enabled, since by that point we know you won't be doing any debugging anyway.

It looks like @nnethercote's Valgrind fix made it into the 3.19 release. Looking around at some distros, I see the following versions of Valgrind packaged:

T-compiler discussed this in a dedicated steering meeting on Zulip. The following action items were noted:

• create list of distros/versions from meeting poll results
• investigate llvm support for v0 mangling
• consider creating a standard test file with various v0 symbols to validate the various demangler implementations

For info, T-compiler held a design meeting 2 weeks ago about updating v0 symbol mangling (on Zulip), specifically

1. Do nothing: external tools just will have to catch up with the changes.
2. Mitigate the problem by implementing more fine-grained graceful degradation as suggested in MCP#737 (at the cost of sometimes longer symbol names and some implementation complexity).

It was decided to go for option 1, therefore closing MCP#737.

Actionables:

• @michaelwoerister will open a PR on the documentation on the v0 policy

(please feel free to add more details if I forgot anything important)

To clarify: the design meeting was about setting a policy for dealing with updates to the v0 mangling scheme (which sometimes are necessary when new language features are added). The meeting was not about switching from legacy- to v0-mangling.

Removing the assignee. If anyone reading this is interested in moving this forward (see previous comment), feel free to self-assign :-)

A job failed! Check out the build log: (web) (plain)

#2 transferring dockerfile: 2.62kB done
#2 DONE 0.0s

#3 [internal] load metadata for docker.io/library/ubuntu:22.04
#3 ERROR: failed to copy: httpReadSeeker: failed open: unexpected status code https://registry-1.docker.io/v2/library/ubuntu/manifests/sha256:67cadaff1dca187079fce41360d5a7eb6f7dcd3745e53c79ad5efd8563118240: 429 Too Many Requests - Server message: toomanyrequests: You have reached your unauthenticated pull rate limit. https://www.docker.com/increase-rate-limit
------
 > [internal] load metadata for docker.io/library/ubuntu:22.04:
------
Dockerfile:1
--------------------
   1 | >>> FROM ubuntu:22.04
   2 |     
   3 |     ARG DEBIAN_FRONTEND=noninteractive
--------------------
ERROR: failed to solve: ubuntu:22.04: failed to resolve source metadata for docker.io/library/ubuntu:22.04: failed to copy: httpReadSeeker: failed open: unexpected status code https://registry-1.docker.io/v2/library/ubuntu/manifests/sha256:67cadaff1dca187079fce41360d5a7eb6f7dcd3745e53c79ad5efd8563118240: 429 Too Many Requests - Server message: toomanyrequests: You have reached your unauthenticated pull rate limit. https://www.docker.com/increase-rate-limit
Command failed. Attempt 2/5:
#0 building with "awesome_rhodes" instance using docker-container driver

#1 [internal] load build definition from Dockerfile
#1 transferring dockerfile: 2.62kB done
#1 DONE 0.0s

#2 [internal] load metadata for docker.io/library/ubuntu:22.04
#2 ERROR: failed to copy: httpReadSeeker: failed open: unexpected status code https://registry-1.docker.io/v2/library/ubuntu/manifests/sha256:67cadaff1dca187079fce41360d5a7eb6f7dcd3745e53c79ad5efd8563118240: 429 Too Many Requests - Server message: toomanyrequests: You have reached your unauthenticated pull rate limit. https://www.docker.com/increase-rate-limit
------
 > [internal] load metadata for docker.io/library/ubuntu:22.04:
------
Dockerfile:1
--------------------
   1 | >>> FROM ubuntu:22.04
   2 |     
   3 |     ARG DEBIAN_FRONTEND=noninteractive
--------------------
ERROR: failed to solve: ubuntu:22.04: failed to resolve source metadata for docker.io/library/ubuntu:22.04: failed to copy: httpReadSeeker: failed open: unexpected status code https://registry-1.docker.io/v2/library/ubuntu/manifests/sha256:67cadaff1dca187079fce41360d5a7eb6f7dcd3745e53c79ad5efd8563118240: 429 Too Many Requests - Server message: toomanyrequests: You have reached your unauthenticated pull rate limit. https://www.docker.com/increase-rate-limit
Command failed. Attempt 3/5:
#0 building with "awesome_rhodes" instance using docker-container driver

#1 [internal] load build definition from Dockerfile
#1 transferring dockerfile: 2.62kB done
#1 DONE 0.0s

#2 [internal] load metadata for docker.io/library/ubuntu:22.04
#2 ERROR: failed to copy: httpReadSeeker: failed open: unexpected status code https://registry-1.docker.io/v2/library/ubuntu/manifests/sha256:67cadaff1dca187079fce41360d5a7eb6f7dcd3745e53c79ad5efd8563118240: 429 Too Many Requests - Server message: toomanyrequests: You have reached your unauthenticated pull rate limit. https://www.docker.com/increase-rate-limit
------
 > [internal] load metadata for docker.io/library/ubuntu:22.04:
------
Dockerfile:1
--------------------
   1 | >>> FROM ubuntu:22.04
   2 |     
   3 |     ARG DEBIAN_FRONTEND=noninteractive
--------------------
ERROR: failed to solve: ubuntu:22.04: failed to resolve source metadata for docker.io/library/ubuntu:22.04: failed to copy: httpReadSeeker: failed open: unexpected status code https://registry-1.docker.io/v2/library/ubuntu/manifests/sha256:67cadaff1dca187079fce41360d5a7eb6f7dcd3745e53c79ad5efd8563118240: 429 Too Many Requests - Server message: toomanyrequests: You have reached your unauthenticated pull rate limit. https://www.docker.com/increase-rate-limit
Command failed. Attempt 4/5:
#0 building with "awesome_rhodes" instance using docker-container driver

#1 [internal] load build definition from Dockerfile
#1 transferring dockerfile: 2.62kB done
#1 DONE 0.0s

#2 [internal] load metadata for docker.io/library/ubuntu:22.04
#2 ERROR: failed to copy: httpReadSeeker: failed open: unexpected status code https://registry-1.docker.io/v2/library/ubuntu/manifests/sha256:67cadaff1dca187079fce41360d5a7eb6f7dcd3745e53c79ad5efd8563118240: 429 Too Many Requests - Server message: toomanyrequests: You have reached your unauthenticated pull rate limit. https://www.docker.com/increase-rate-limit
------
 > [internal] load metadata for docker.io/library/ubuntu:22.04:
------
Dockerfile:1
--------------------
   1 | >>> FROM ubuntu:22.04
   2 |     
   3 |     ARG DEBIAN_FRONTEND=noninteractive
--------------------
ERROR: failed to solve: ubuntu:22.04: failed to resolve source metadata for docker.io/library/ubuntu:22.04: failed to copy: httpReadSeeker: failed open: unexpected status code https://registry-1.docker.io/v2/library/ubuntu/manifests/sha256:67cadaff1dca187079fce41360d5a7eb6f7dcd3745e53c79ad5efd8563118240: 429 Too Many Requests - Server message: toomanyrequests: You have reached your unauthenticated pull rate limit. https://www.docker.com/increase-rate-limit
Command failed. Attempt 5/5:
#0 building with "awesome_rhodes" instance using docker-container driver

#1 [internal] load build definition from Dockerfile
#1 transferring dockerfile: 2.62kB done
#1 DONE 0.0s

#2 [internal] load metadata for docker.io/library/ubuntu:22.04
#2 ERROR: failed to copy: httpReadSeeker: failed open: unexpected status code https://registry-1.docker.io/v2/library/ubuntu/manifests/sha256:67cadaff1dca187079fce41360d5a7eb6f7dcd3745e53c79ad5efd8563118240: 429 Too Many Requests - Server message: toomanyrequests: You have reached your unauthenticated pull rate limit. https://www.docker.com/increase-rate-limit
------
 > [internal] load metadata for docker.io/library/ubuntu:22.04:
------
Dockerfile:1
--------------------
   1 | >>> FROM ubuntu:22.04
   2 |     
   3 |     ARG DEBIAN_FRONTEND=noninteractive
--------------------
ERROR: failed to solve: ubuntu:22.04: failed to resolve source metadata for docker.io/library/ubuntu:22.04: failed to copy: httpReadSeeker: failed open: unexpected status code https://registry-1.docker.io/v2/library/ubuntu/manifests/sha256:67cadaff1dca187079fce41360d5a7eb6f7dcd3745e53c79ad5efd8563118240: 429 Too Many Requests - Server message: toomanyrequests: You have reached your unauthenticated pull rate limit. https://www.docker.com/increase-rate-limit
The command has failed after 5 attempts.
##[error]Process completed with exit code 1.
Post job cleanup.